Skip to content

fix(v2): let an explicit options.password win over the password shorthand - #121

Merged
tian-lan-landing merged 2 commits into
mainfrom
fix/v2-password-precedence-parity
Sep 9, 2026
Merged

fix(v2): let an explicit options.password win over the password shorthand#121
tian-lan-landing merged 2 commits into
mainfrom
fix/v2-password-precedence-parity

Conversation

@tian-lan-landing

@tian-lan-landing tian-lan-landing commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

password is shorthand for the contract field options.password. This SDK let the shorthand win a conflict; ade-python lets the explicit field win. Same call, different password, depending on the language — the loser surfaced only as a 422 encrypted_pdf_wrong_password naming no cause.

Aligns on options.password wins. Paired with landing-ai/ade-python#160, which documents the same rule and pins it with tests.

  • precedence tests the value, not key presence: {password: undefined} is JS for "absent" and JSON.stringify drops the key, so a presence test would suppress the shorthand and erase the key, sending a locked PDF with no password at all
  • drops the fallback that wrote a top-level password form field — the spec has declared no such field since 2026-07-16, so the gateway dropped it and the key was lost silently
  • options now goes through one helper rejecting a non-object on both the string and object branch, mirroring coerceSchema

Fixes:

🤖 Generated with Claude Code

…hand

`password` is shorthand for the contract field `options.password` -- the spec
declares the password inside `options` and nowhere else. `buildParseForm`
spread the shorthand in last, so it overwrote a password the caller had written
into `options`, while ade-python's `_build_parse_body` resolves the same
conflict the other way. The same call therefore decrypted with a different
password depending on the language, and the losing one surfaced only as a 422
`encrypted_pdf_wrong_password` naming no cause.

Give the explicit field precedence, matching ade-python, and test the value
rather than key presence: `{password: undefined}` is how JS spells "absent" and
`JSON.stringify` drops the key, so a presence test would suppress the shorthand
and then erase the key, sending a locked PDF with no password at all.

Also drop the fallback that wrote a top-level `password` form field when
`options` was a string that did not parse -- the contract declares no such
field, so the gateway dropped it and the key was lost silently. `options` now
goes through one helper that rejects a non-object on both the string and the
object branch, mirroring `coerceSchema`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 9, 2026 01:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation consistently handles both parse routes and is well covered by focused tests.

Pull request overview

Aligns V2 parse password precedence with the Python SDK while validating serialized options.

Changes:

  • Makes explicit options.password override shorthand password.
  • Rejects malformed or non-object options.
  • Adds documentation and regression tests.
File summaries
File Description
src/resources/v2/parse.ts Implements precedence and options validation.
tests/api-resources/v2/v2.test.ts Covers serialization, precedence, and invalid inputs.
README.md Documents password behavior.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…tened paths

Review follow-ups on the precedence fix.

`opts?.['password']` walks the prototype chain, so a polluted
`Object.prototype.password` made every options object look like it already carried
one: the shorthand suppressed, nothing serialized, and a locked PDF shipped with no
password at all. Read it as an own property.

The rest is coverage and wording for behavior the first commit changed but did not
pin: an explicit `options.password: null` silencing the shorthand, a malformed
`options` rejected on a call with no password in it (`options` is coerced
unconditionally now), and the object branch rejecting an array. Both READMEs now
state the `null` rule and the coercion, `Unsupported options type` names an array as
`array` rather than `object`, and the rejection cases are table-driven so one bad
input no longer hides the rest.

Also fixes tests that pinned `pages: '1-2'`, which the spec types `integer[] | null`
under `additionalProperties: false` -- they documented a request the gateway rejects.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 9, 2026 04:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation matches the documented behavior and is well covered by targeted tests.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@tian-lan-landing
tian-lan-landing merged commit bab73d6 into main Sep 9, 2026
7 checks passed
tian-lan-landing added a commit to landing-ai/ade-python that referenced this pull request Sep 9, 2026
…ct options (#160)

* docs(v2): pin and document the parse password precedence rule

`password` is shorthand for the contract field `options["password"]`, and
`_build_parse_body` already lets the explicit field win the tie. That rule
lived only in a comment and a docstring, and only the sync route's dict branch
had a test for it -- ade-typescript had shipped the opposite rule, so the same
call decrypted with a different password depending on the SDK.

- pin the rule on the `options` JSON-string branch and on `parse_jobs.create`,
  the two paths that had no conflict coverage
- state the cross-SDK contract next to the code that implements it, replacing
  a vaguer note that read as being about folding rather than precedence
- document `password` and the rule in the README, which had no encrypted-PDF
  section at all

No behavior change; ade-typescript moves to this rule to match.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(v2): reject an options value that is not a JSON object

`_build_parse_body` coerced `options` with `dict(json.loads(...))`, and `dict()`
accepts any pair-sequence -- so a JSON *array* silently became the options dict:

    options='[["password", "sneaky"]]', password="kw"  ->  {"password": "sneaky"}

The smuggled key then won the precedence tie against the caller's own `password`
argument, which is the exact silent substitution that rule exists to prevent.
Non-dict scalars fared no better: `TypeError: 'int' object is not iterable`, or a
`ValueError` about a "dictionary update sequence" -- neither naming the field.

Route `options` through `_coerce_options`, mirroring `coerce_schema_to_dict` in
`lib/schema_utils.py`: decode a string, require an object, and name the field
when it is not one. Malformed JSON still surfaces as the `ValueError` that
`json.loads` raises, exactly as it does for `schema`.

Found while aligning precedence with ade-typescript, which rejects the same
inputs (landing-ai/ade-typescript#121).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(v2): document the options tightening and cover its untested paths

Review follow-ups on the precedence work.

`_coerce_options` narrowed the non-string branch from `dict()` to `Mapping`, not
just the JSON-string branch -- and that half was undocumented and untested. It is
deliberate: `dict()` accepts any pair-sequence in list form too, so `[["password",
"x"]]` smuggled a password exactly like its string twin did. The docstring now says
so, including the one place this stops mirroring `coerce_schema_to_dict` (that
helper takes a pydantic model; `options` never advertised one), and the rejection
test covers both forms plus the previously uncovered `Unsupported options type`
branch.

Also: drop the `str` arm of the final serialization, which `_coerce_options` made
unreachable while still reading as "a pre-serialized string is forwarded verbatim"
-- the opposite of what the code does; add async precedence coverage, since all
four call sites share `_build_parse_body` but only the sync two were pinned; make
the rejection cases table-driven so one bad input no longer hides the rest; and fix
a README example that imported `os` but used an undefined `Path`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(v2): type options as accepting a JSON string, and name both rejection errors

Copilot review on #160.

The docs this branch added advertise a JSON string for `options`, and
`_coerce_options` accepts one, but every public annotation still said
`Optional[Mapping[str, object]]` -- so a typed caller could not use the advertised
form without the `type: ignore` the tests carried. Widen all six sync/async parse
and job signatures, matching how extract already types its coercible sibling
(`schema: Union[str, Mapping[str, object], Type[BaseModel]]`), and drop the ignores
that are no longer needed. griffe reports no breakage: the change is additive.

The same docs also promised `TypeError` for anything unacceptable, which is wrong
for the case most likely to hit it -- malformed JSON propagates
`json.JSONDecodeError` from `json.loads`, as this branch's own test asserts. Name
both paths instead, in the two docstrings and the README.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing added a commit to landing-ai/ade-python that referenced this pull request Sep 9, 2026
…e aliases

check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring
prompt scoped routes too, and said nothing about which FIELDS back a /v2
request — which is how the parse password went wrong: the spec has
declared it at options.password and nowhere else since 2026-07-16, the
same spec still declares a top-level `password` on /v1/ade/parse*, and
both SDKs ended up with a hand-written top-level shorthand whose
tie-break was written down nowhere. They picked opposite ones, so one
call sent a different password per language (#160,
landing-ai/ade-typescript#121).

- the prompt's SCOPE block now scopes fields as well as routes, names
  both traps (a nested spec field is not a top-level one; a /v1
  top-level field is not a /v2 field), and tells the AI pass to leave a
  new convenience alias to a maintainer rather than invent one.
- CONTRIBUTING records the two aliases that exist (`password` ->
  options.password, `strict` -> options.strict), the precedence rule for
  each, the language-specific traps in both SDKs, and — plainly — that
  nothing in CI checks any of it, so a PR touching a non-spec top-level
  param has to be diffed against the other repo by hand.

A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was
built and dropped as more machinery than the problem warrants: it could
not verify a tie-break from code anyway, only that a rule had been
written down somewhere. The risk stays with review.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing added a commit that referenced this pull request Sep 9, 2026
…e aliases

check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring
prompt scoped routes too, and said nothing about which FIELDS back a /v2
request — which is how the parse password went wrong: the spec has
declared it at options.password and nowhere else since 2026-07-16, the
same spec still declares a top-level `password` on /v1/ade/parse*, and
both SDKs ended up with a hand-written top-level shorthand whose
tie-break was written down nowhere. They picked opposite ones, so one
call sent a different password per language (#121,
landing-ai/ade-python#160).

- the prompt's SCOPE block now scopes fields as well as routes, names
  both traps (a nested spec field is not a top-level one; a /v1
  top-level field is not a /v2 field), and tells the AI pass to leave a
  new convenience alias to a maintainer rather than invent one.
- CONTRIBUTING records the two aliases that exist (`password` ->
  options.password, `strict` -> options.strict), the precedence rule for
  each, the language-specific traps in both SDKs, and — plainly — that
  nothing in CI checks any of it, so a PR touching a non-spec top-level
  param has to be diffed against the other repo by hand.

A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was
built and dropped as more machinery than the problem warrants: it could
not verify a tie-break from code anyway, only that a rule had been
written down somewhere. The risk stays with review.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing added a commit to landing-ai/ade-python that referenced this pull request Sep 9, 2026
…e aliases

check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring
prompt scoped routes too, and said nothing about which FIELDS back a /v2
request — which is how the parse password went wrong: the spec has
declared it at options.password and nowhere else since 2026-07-16, the
same spec still declares a top-level `password` on /v1/ade/parse*, and
both SDKs ended up with a hand-written top-level shorthand whose
tie-break was written down nowhere. They picked opposite ones, so one
call sent a different password per language (#160,
landing-ai/ade-typescript#121).

- the prompt's SCOPE block now scopes fields as well as routes, names
  both traps (a nested spec field is not a top-level one; a /v1
  top-level field is not a /v2 field), and tells the AI pass to leave a
  new convenience alias to a maintainer rather than invent one.
- CONTRIBUTING records the two aliases that exist (`password` ->
  options.password, `strict` -> options.strict), the precedence rule for
  each, the language-specific traps in both SDKs, and — plainly — that
  nothing in CI checks any of it, so a PR touching a non-spec top-level
  param has to be diffed against the other repo by hand.

A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was
built and dropped as more machinery than the problem warrants: it could
not verify a tie-break from code anyway, only that a rule had been
written down somewhere. The risk stays with review.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing added a commit to landing-ai/ade-python that referenced this pull request Sep 9, 2026
…e aliases (#161)

check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring
prompt scoped routes too, and said nothing about which FIELDS back a /v2
request — which is how the parse password went wrong: the spec has
declared it at options.password and nowhere else since 2026-07-16, the
same spec still declares a top-level `password` on /v1/ade/parse*, and
both SDKs ended up with a hand-written top-level shorthand whose
tie-break was written down nowhere. They picked opposite ones, so one
call sent a different password per language (#160,
landing-ai/ade-typescript#121).

- the prompt's SCOPE block now scopes fields as well as routes, names
  both traps (a nested spec field is not a top-level one; a /v1
  top-level field is not a /v2 field), and tells the AI pass to leave a
  new convenience alias to a maintainer rather than invent one.
- CONTRIBUTING records the two aliases that exist (`password` ->
  options.password, `strict` -> options.strict), the precedence rule for
  each, the language-specific traps in both SDKs, and — plainly — that
  nothing in CI checks any of it, so a PR touching a non-spec top-level
  param has to be diffed against the other repo by hand.

A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was
built and dropped as more machinery than the problem warrants: it could
not verify a tie-break from code anyway, only that a rule had been
written down somewhere. The risk stays with review.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing added a commit that referenced this pull request Sep 9, 2026
…e aliases (#122)

check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring
prompt scoped routes too, and said nothing about which FIELDS back a /v2
request — which is how the parse password went wrong: the spec has
declared it at options.password and nowhere else since 2026-07-16, the
same spec still declares a top-level `password` on /v1/ade/parse*, and
both SDKs ended up with a hand-written top-level shorthand whose
tie-break was written down nowhere. They picked opposite ones, so one
call sent a different password per language (#121,
landing-ai/ade-python#160).

- the prompt's SCOPE block now scopes fields as well as routes, names
  both traps (a nested spec field is not a top-level one; a /v1
  top-level field is not a /v2 field), and tells the AI pass to leave a
  new convenience alias to a maintainer rather than invent one.
- CONTRIBUTING records the two aliases that exist (`password` ->
  options.password, `strict` -> options.strict), the precedence rule for
  each, the language-specific traps in both SDKs, and — plainly — that
  nothing in CI checks any of it, so a PR touching a non-spec top-level
  param has to be diffed against the other repo by hand.

A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was
built and dropped as more machinery than the problem warrants: it could
not verify a tie-break from code anyway, only that a rule had been
written down somewhere. The risk stays with review.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants